Skip to content

Fix Codex Fast mode accounting from current trace records - #2519

Open
Anri-Lombard wants to merge 1 commit into
steipete:mainfrom
Anri-Lombard:agent/fix-codex-priority-trace
Open

Fix Codex Fast mode accounting from current trace records#2519
Anri-Lombard wants to merge 1 commit into
steipete:mainfrom
Anri-Lombard:agent/fix-codex-priority-trace

Conversation

@Anri-Lombard

Copy link
Copy Markdown

Summary

CodexBar currently misses Fast/Priority mode for current Codex/Luna sessions. Those sessions record priority usage in Submission trace rows with service_tier: Some(Some("priority")), while the scanner only recognized legacy websocket request rows.

This causes current Fast mode sessions to be classified as standard-cost usage.

Changes

  • Include current priority Submission rows in bounded and incremental SQLite scans.
  • Parse the submission turn ID and thread ID without retaining prompt text.
  • Keep legacy websocket trace parsing unchanged.
  • Add parser and SQLite regression coverage.
  • Regenerate the parser hash required by the repository checks.

Validation

  • swift test --filter CostUsageScannerCodexPriorityTests — 21 passed
  • swift test --filter CostUsageScannerPriorityTests — 16 passed
  • make check — passed, including SwiftFormat/SwiftLint and repository gates
  • make test — passed, 758 selections across 64 groups

Observed local evidence: Codex logs contain service_tier: Some(Some("priority")) for current sessions, while the pre-fix CodexBar cache classified the same session as standard.

@Anri-Lombard
Anri-Lombard marked this pull request as ready for review July 30, 2026 19:03

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0b94e5806

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +572 to +573
guard body.contains(#"service_tier: Some(Some("priority"))"#),
let submissionRange = body.range(of: "Submission sub=Submission {")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restrict the priority marker to thread settings

When a standard-mode user prompt itself contains service_tier: Some(Some("priority"))—for example, while debugging or pasting a trace—the serialized Submission row includes that text in op: UserInput even though its actual ThreadSettingsOverrides uses the default tier. This whole-body check therefore marks that standard turn as Fast and overstates its cost; inspect the actual thread_settings field rather than accepting the marker anywhere in the prompt-bearing row.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jul 31, 2026
@clawsweeper

clawsweeper Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 31, 2026, 7:02 PM ET / 23:02 UTC.

ClawSweeper review

What this changes

The PR recognizes current Codex Submission trace rows as priority usage during SQLite scans and adds parser and database regression tests.

Merge readiness

Blocked until real behavior proof is added - 7 items remain

Keep this PR open. The requested current-trace support is not on main, but the proposed Submission parser still treats a priority-looking string anywhere in the prompt-bearing row as Fast mode, so it can overstate usage costs for standard sessions; it also still needs inspectable after-fix runtime proof.

Priority: P2
Reviewed head: d0b94e5806d8ada2a0ed3d42ff0b2a8a7ac1684b

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The implementation is focused, but the unresolved false-positive defect and missing real behavior proof make it unready to merge.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR reports local observations and passing tests, but it contains no inspectable redacted terminal capture, runtime log, recording, or live output showing a real current Submission trace classified as Priority after the change. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR reports local observations and passing tests, but it contains no inspectable redacted terminal capture, runtime log, recording, or live output showing a real current Submission trace classified as Priority after the change. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 6 items Current main behavior: Current main only accepts legacy websocket response.create rows with JSON service_tier equal to priority; it has no Submission-row parsing path, so the central capability is not already implemented.
Patch false positive: The proposed Submission parser first searches the entire raw row for the priority marker before locating Submission; a standard user prompt containing that literal trace text passes this guard even when its actual thread_settings tier is not priority.
Regression coverage gap: The added tests exercise a positive thread_settings row but do not cover a standard Submission whose prompt text contains the priority marker, leaving the false-positive path unprotected.
Findings 1 actionable finding [P2] Scope the marker to thread_settings
Security None None.

How this fits together

CodexBar scans local Codex trace records stored in a SQLite log cache to identify priority turns. The resulting turn metadata is joined with usage data to calculate the menu bar app’s Codex usage and cost display.

flowchart LR
  A[Codex trace records] --> B[SQLite log cache]
  B --> C[Priority trace scanner]
  C --> D[Priority turn metadata]
  D --> E[Usage and cost matching]
  E --> F[Menu bar usage display]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR reports local observations and passing tests, but it contains no inspectable redacted terminal capture, runtime log, recording, or live output showing a real current Submission trace classified as Priority after the change. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Scope the marker to `thread_settings` (P2) - The new guard searches the whole Submission record for the priority marker before parsing its fields. Because op: UserInput contains raw prompt text, a standard session that pastes this trace string is recorded as Fast; inspect the actual thread_settings segment and add the corresponding negative regression test.
  • Resolve merge risk (P1) - A standard Submission row can be counted as Fast/Priority when its user prompt happens to contain the raw priority-marker text, inflating displayed usage costs.
  • Resolve merge risk (P1) - The PR body reports local observations and test commands but provides no inspectable after-fix runtime artifact, so reviewers cannot verify that a real current trace is classified correctly.
  • Complete next step (P2) - A bounded repair can correct the field boundary and regression coverage, but the contributor must still supply real after-fix evidence before merge.
  • Improve patch quality - Parse only the Submission thread_settings field and add a negative prompt-marker regression test.
  • Improve patch quality - Add a redacted terminal capture or runtime log that shows a real current Submission trace classified as Priority after the fix.

Findings

  • [P2] Scope the marker to thread_settingsSources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CodexPriority.swift:572-573
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 3 files affected; 74 added, 6 removed The patch is narrowly focused on the trace parser, its generated integrity hash, and focused regression coverage.

Merge-risk options

Maintainer options:

  1. Scope the Submission-tier parser (recommended)
    Parse the thread_settings field rather than searching the whole prompt-bearing row, and add a regression test for a standard turn whose prompt contains the marker text.
  2. Pause until source and proof are updated
    Keep the PR open without merging if the trace grammar cannot be safely field-scoped or if a redacted real-trace result cannot be supplied.

Technical review

Best possible solution:

Parse the actual thread_settings portion of Submission rows, add a standard-mode prompt-marker regression case, and attach a redacted live log or terminal capture showing a real current Submission row classified as Priority after the fix.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: a Submission row with the marker inside op: UserInput but without priority thread_settings passes the new whole-row guard and is returned as a priority turn. The missing negative regression test should establish this against the PR branch.

Is this the best way to solve the issue?

No: recognizing Submission rows is the right boundary, but searching the whole prompt-bearing record is not safe. Restrict the detection to the serialized thread_settings field and retain the existing legacy websocket parser unchanged.

Full review comments:

  • [P2] Scope the marker to thread_settingsSources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CodexPriority.swift:572-573
    The new guard searches the whole Submission record for the priority marker before parsing its fields. Because op: UserInput contains raw prompt text, a standard session that pastes this trace string is recorded as Fast; inspect the actual thread_settings segment and add the corresponding negative regression test.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 8ef86077e70a.

Labels

Label justifications:

  • P2: Incorrect priority classification affects cost accounting but the evidence does not indicate an urgent availability or security incident.
  • merge-risk: 🚨 other: Merging the current whole-row marker check can misclassify standard sessions and overstate users’ Fast/Priority cost usage.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports local observations and passing tests, but it contains no inspectable redacted terminal capture, runtime log, recording, or live output showing a real current Submission trace classified as Priority after the change. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Acceptance criteria:

  • [P1] swift test --filter CostUsageScannerCodexPriorityTests.
  • [P1] make check.
  • [P1] make test.

What I checked:

Likely related people:

  • Peter Steinberger: Available file history attributes six commits on the priority-scanner file to Peter Steinberger, the largest contribution count in the local history. (role: primary historical contributor; confidence: medium; commits: b1170d83963d, 8ef86077e70a; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CodexPriority.swift)
  • Milan Mijatovic: The latest available commit touching the priority-scanner file is Milan Mijatovic’s July 29, 2026 update. (role: recent area contributor; confidence: medium; commits: 64fd710c59fc; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CodexPriority.swift)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-07-31T16:24:59.505Z sha d0b94e5 :: needs real behavior proof before merge. :: [P2] Restrict the priority marker to thread settings

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. label Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant